-
-
Notifications
You must be signed in to change notification settings - Fork 489
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix .cjs files not showing up in bundle analyzer #512
Conversation
Btw. I signed CLA. You can re-run your CI. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Thanks! :) I would appreciate if it could be released in the nearby future. |
Looks good, thanks! Are you able to create a small test to verify this won't regress in the future? Also a mention of this change in changelog would be nice 😊 |
Hi @Rush are you still interested in getting this PR done? We'd need this PR to be rebased on top of latest |
@valscion Hello, can we resolve this PR? Because a got a lot of questions why |
Yeah a PR which adds a minimal test case showing that .cjs files work, adds a changelog entry and does this same change will be accepted. |
Okey, I will resend it 👍 |
Sorry I don't have any bandwidth to work on it. One solution to consider would be to merge this and create a follow up issue with a test case. I've been using my .cjs branch for almost two years now :-) |
Yeah now that this PR is up-to-date then adding some test coverage should hopefully be quite simple. Would you @Rush have bandwidth to create a changelog entry for this change still? |
@valscion let me know if this is what you had in mind |
@valscion Added a test case |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great, thanks!
This has now been released as part of v4.10.2 |
I compile my server-side code to .cjs files. I was surprised to find the bundle analyzer view to be completely empty.
After debugging it turned out that .cjs filename was not being handled.
I think in general all assets should be included in the analysis but .cjs is good for now!
Could you release this patch on npm as well?